fix: add content-type check for non-JSON responses in fetchExternal#825
fix: add content-type check for non-JSON responses in fetchExternal#825Pavan-Microsoft wants to merge 2 commits into
Conversation
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
This PR tightens HttpClient.fetchExternal() response validation by verifying the response Content-Type indicates JSON before attempting to parse, aiming to improve runtime safety and error clarity for non-API endpoints (e.g., /.auth/me).
Changes:
- Add a
Content-Typeheader check infetchExternal()and throw a descriptive error for non-JSON responses. - Preserve existing
response.okhandling while improving the failure mode for unexpected response bodies.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new Error(`${config.method || 'GET'} ${url} failed: ${response.statusText}`); | ||
| } | ||
| const contentType = response.headers.get('content-type') || ''; | ||
| if (!contentType.includes('application/json')) { |
There was a problem hiding this comment.
The JSON detection is overly strict: valid JSON responses may use vendor/problem types like application/problem+json or application/vnd.api+json, and header values are case-insensitive. Consider normalizing to lowercase and accepting application/json or any +json media type (per RFC 6839) to avoid false negatives.
| if (!contentType.includes('application/json')) { | |
| const normalizedContentType = contentType.toLowerCase(); | |
| const mediaType = normalizedContentType.split(';', 1)[0].trim(); | |
| const isJsonResponse = mediaType === 'application/json' || mediaType.endsWith('+json'); | |
| if (!isJsonResponse) { |
| if (!response.ok) { | ||
| throw new Error(`${config.method || 'GET'} ${url} failed: ${response.statusText}`); | ||
| } | ||
| const contentType = response.headers.get('content-type') || ''; |
There was a problem hiding this comment.
This error message can be unhelpful when the server omits the Content-Type header (you'll end up with returned non-JSON response ()). Consider distinguishing between a missing header and a non-JSON header value so the thrown error is actionable during debugging.
| const contentType = response.headers.get('content-type') || ''; | |
| const contentType = response.headers.get('content-type'); | |
| if (!contentType) { | |
| throw new Error(`${config.method || 'GET'} ${url} returned response missing Content-Type header`); | |
| } |
| const contentType = response.headers.get('content-type') || ''; | ||
| if (!contentType.includes('application/json')) { | ||
| throw new Error(`${config.method || 'GET'} ${url} returned non-JSON response (${contentType})`); | ||
| } | ||
| return response.json(); |
There was a problem hiding this comment.
PR description says the HttpClient now ensures all HTTP responses are JSON before parsing, but this validation is only added to fetchExternal(); the core request() path still calls response.json() unconditionally. Either broaden the check to request() as well or adjust the PR description to match the actual behavior.
GitHub Actions
Inactive
Purpose
This pull request adds stricter response validation to the
HttpClientutility by ensuring that all HTTP responses are JSON before attempting to parse them. This helps prevent runtime errors and improves error handling.Improvements to error handling:
src/App/src/utils/httpClient.ts: Added a check to verify that thecontent-typeheader includesapplication/jsonbefore parsing the response, and throw a descriptive error if not.Does this introduce a breaking change?
Golden Path Validation
Deployment Validation